pytest tmp_dir fixture #706

loomlike · 2019-04-03T04:57:24Z

Description

Introduce tmp_dir fixture
Refactor tests to use the fixture
Fix a bug in movielens load_spark_df - could throw an error when item_df is None. Now it handles the case.
Make movielens loader require at least two columns in the header, user and item

Related Issues

#701

Checklist:

My code follows the code style of this project, as detailed in our contribution guidelines.
I have added tests.
I have updated the documentation accordingly.

Refactor tests to use the fixture Fix movielens bug

miguelgfierro · 2019-04-03T10:39:29Z

tests/conftest.py

@@ -23,6 +23,15 @@
    pass  # so the environment without spark doesn't break


+@pytest.fixture
+def tmp_dir(tmp_path_factory):


a note of naming, the pytest temporary dir is called tmpdir which is almost the same as ours. Maybe we should rename it to avoid users mistake pytest fixture with our fixture. Maybe temporary_dir?

good suggestion. maybe even more specific, something like fn_scope_tmpdir?

or does it sound like some kind of function because of some name convention?

i'm missing where tmp_path_factory is coming from?

@gramhagen it is pytest's fixture: https://docs.pytest.org/en/latest/tmpdir.html

name changed to just tmp, named after linux tmp dir. I picked that for simplicity, but if you think it is not very descriptive, feel free to let me know or give any suggestions. temporary_dir looks (visually) unbalanced to me...haha

one question, do we foresee any case where we want to have a directory per (pytest) module or even per (pytest) session?

We can make fixtures for them when we need, like sess_tmp and module_tmp.

miguelgfierro · 2019-04-03T10:40:39Z

tests/smoke/test_movielens.py

@@ -50,53 +49,50 @@ def test_load_pandas_df(
    title_example,
    genres_example,
    year_example,
+    tmp_dir,


there is no need to import this since it is a fixture that it is in conftest, see this: https://github.com/Microsoft/Recommenders/blob/master/tests/conftest.py#L4

ohhhh good! will update

ah, nvm. it means you don't need to do import your_fixture_module. You still need to pass the fixture object as an argument. I think the comments contain misleading contents.

good point, yeah, what you said is more detailed. It would be nice to have it in the code

miguelgfierro · 2019-04-03T10:45:59Z

tests/conftest.py

@@ -23,6 +23,15 @@
    pass  # so the environment without spark doesn't break


+@pytest.fixture


I also like the context manager option, maybe it can be implemented as we are discussing here: #701 (comment)

Let me try out that. looks promising.

checked and turns out normal fixture does the same thing since newer version of pytest.

Since pytest-3.0, fixtures using the normal fixture decorator can use a yield statement to provide fixture values and execute teardown code, exactly like yield_fixture in previous versions. Marking functions as yield_fixture is still supported, but deprecated and should not be used in new code.

good, so if we use the fixture as you program it using: with temp_dir: is the folder erased after the test function is finished?

yes. Even try/finally does remove the dir too, but using the context manager made the code simpler and give us peace of mind :-)

Change to use context manager Change fixture name from tmp_dir to tmp

yueguoguo · 2019-04-04T09:10:50Z

reco_utils/dataset/movielens.py

 WARNING_HAVE_SCHEMA_AND_HEADER = """Both schema and header are provided.
    The header argument will be ignored."""
 ERROR_MOVIE_LENS_SIZE = "Invalid data size. Should be one of {100k, 1m, 10m, or 20m}"
-ERROR_NO_HEADER = "No header (schema) information"
+ERROR_NO_HEADER = "No header (schema) information. At least user and movie column names should be provided"


Really like these constant string of error/warning message. Do we later want to consolidate this into somewhere in common/constants? Or, at least, adopt this practice in other utility codes.

If we need, sure why not. We can even generalize further, e.g. use obj.__name__ or to accept names as args.

yueguoguo · 2019-04-04T09:14:20Z

reco_utils/dataset/movielens.py

-    if movie_col is not None:
-        item_header.append(movie_col)
-        usecols.append(0)
+    if title_col is None and genres_col is None and year_col is None:


Looks like the check now is different from before? The previous checks the movie_col and return None if it does not exist, whilst now it checks other three columns.

yeah, made movie_col as a default (must-have) column, checking earlier as: if header is None or len(header) < 2: (2 is the first two columns, user and movie), because there is no point to load only user ids.

yueguoguo · 2019-04-04T09:15:51Z

tests/conftest.py

@@ -23,6 +23,12 @@
    pass  # so the environment without spark doesn't break


+@pytest.fixture
+def tmp(tmp_path_factory):


This looks nice and clean ❤️

kudos to @miguelgfierro @anargyri!

* pytest tmp_dir fixture Refactor tests to use the fixture Fix movielens bug * Update conftest description

pytest tmp_dir fixture

c61dc6c

Refactor tests to use the fixture Fix movielens bug

loomlike requested review from miguelgfierro and anargyri April 3, 2019 04:57

loomlike requested a review from yueguoguo as a code owner April 3, 2019 04:57

miguelgfierro reviewed Apr 3, 2019

View reviewed changes

anargyri approved these changes Apr 3, 2019

View reviewed changes

tmp fixture to use context manager

94c83a3

Change to use context manager Change fixture name from tmp_dir to tmp

loomlike requested a review from miguelgfierro April 4, 2019 00:52

yueguoguo approved these changes Apr 4, 2019

View reviewed changes

Update conftest description

570500e

loomlike merged commit e421c00 into staging Apr 4, 2019

loomlike deleted the jumin/pytest_tmp_dir branch April 4, 2019 19:38

yueguoguo pushed a commit that referenced this pull request Sep 9, 2019

pytest tmp_dir fixture (#706)

10c2f24

* pytest tmp_dir fixture Refactor tests to use the fixture Fix movielens bug * Update conftest description

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pytest tmp_dir fixture #706

pytest tmp_dir fixture #706

loomlike commented Apr 3, 2019

miguelgfierro Apr 3, 2019

loomlike Apr 3, 2019

loomlike Apr 3, 2019

gramhagen Apr 3, 2019

loomlike Apr 4, 2019

loomlike Apr 4, 2019

miguelgfierro Apr 4, 2019

loomlike Apr 4, 2019

miguelgfierro Apr 3, 2019

loomlike Apr 3, 2019

loomlike Apr 4, 2019

miguelgfierro Apr 4, 2019

miguelgfierro Apr 3, 2019

loomlike Apr 3, 2019

loomlike Apr 4, 2019

miguelgfierro Apr 4, 2019

loomlike Apr 4, 2019

yueguoguo Apr 4, 2019

loomlike Apr 4, 2019

yueguoguo Apr 4, 2019

loomlike Apr 4, 2019

yueguoguo Apr 4, 2019

loomlike Apr 4, 2019

		@@ -23,6 +23,15 @@
		pass # so the environment without spark doesn't break


		@pytest.fixture

pytest tmp_dir fixture #706

pytest tmp_dir fixture #706

Conversation

loomlike commented Apr 3, 2019

Description

Related Issues

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment